Support record positional construction inlay hints #4447
Conversation
| mkCodeAction :: [Extension] -> Int -> Command |? CodeAction | ||
| mkCodeAction exts uid = InR CodeAction | ||
| { _title = mkTitle exts | ||
| { _title = mkTitle exts -- TODO: `Expand positional record` without NamedFieldPuns if RecordInfoApp |
There was a problem hiding this comment.
The existing codeActionProvider only needs "the list of extensions, and a int to allow us to resolve it later", but here mkTitle needs to be decided based on the RecordInfo how to generate it; If the RecordInfo is obtained here, does it not conform to the idea of resolve?
should I adjust the RecordInfo to make two codeActionProvider? Or is there any better way?
There was a problem hiding this comment.
I think we probably don't want to get the RecordInfo here if we can avoid it, that's the expensive bit, right?
I would have thought that the code action doesn't apply it all if it's a RecordInfoApp. Isn't the code action only for record wildcards?
There was a problem hiding this comment.
Yes, I also think we should avoid getting RecordInfo before resolve.
Since RecordInfoApp is a constructor of RecordInfo, so it is also processed in the code action provider that process RecordInfo.
I thought that the construction of positional record can also have code actions, which should also be considered?
When the code action resolves, we can get the RecordInfo, so a useless extension RecordWildCards will not actually be inserted if RecordInfo is RecordInfoApp.
The only problem with the current code is that the title of the code action will display Expand record wildcard (needs extension: NamedFieldPuns) (when the extension does not exist in current file), no matter what it is RecordInfo.
There was a problem hiding this comment.
I guess what I'm saying is that we shouldn't have any code actions where the RecordInfo is RecordInfoApp, since RecordInfoApp can't be a wildcard, and that's what the code action applies to?
jetjinser
changed the title
that deleted by mistake
jetjinser
force-pushed
the
inlay-hints-positional-record
branch
from
056a622 to
b7f110f
Compare
| , _kind = Nothing -- neither a type nor a parameter | ||
| , _textEdits = Just (maybeToList te) -- same as CodeAction | ||
| , _tooltip = Just $ InL "Expand positional record" -- same as CodeAction | ||
| , _paddingLeft = Nothing -- padding after dotdot |
There was a problem hiding this comment.
comment is not quite right?
| getFields :: HsExpr GhcTc -> [LHsExpr GhcTc] -> Maybe RecordAppExpr | ||
| getFields (HsApp _ (unLoc -> (XExpr (ConLikeTc (conLikeFieldLabels -> fls) _ _))) _) _ | ||
| | null fls = Nothing | ||
| getFields (HsApp _ constr@(unLoc -> (XExpr (ConLikeTc (conLikeFieldLabels -> fls) _ _))) arg) args |
There was a problem hiding this comment.
I think you can just guard this case with | not (null fls) and then delete the case above, since the fallthrough case will handle it?
| mkCodeAction :: [Extension] -> Int -> Command |? CodeAction | ||
| mkCodeAction exts uid = InR CodeAction | ||
| { _title = mkTitle exts | ||
| { _title = mkTitle exts -- TODO: `Expand positional record` without NamedFieldPuns if RecordInfoApp |
There was a problem hiding this comment.
I think we probably don't want to get the RecordInfo here if we can avoid it, that's the expensive bit, right?
I would have thought that the code action doesn't apply it all if it's a RecordInfoApp. Isn't the code action only for record wildcards?
|
Let's merge this! |
michaelpj
added
merge me
3 participants
Resolves #4391, resolves #4212.